Uniformize kwargs for Idefics/2 processors #32568
Conversation
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
| # for BC | ||
| if text is None: | ||
| # if the user didn't specify text=text in the call, we assume they want to use the old behavior | ||
| # with text (previously prompts) as a first argument | ||
| warnings.warn( | ||
| "The use of `text` as the first argument will be deprecated in the future. `images` is now the first argument." | ||
| "The first given argument will be considered as `prompts` in the old behavior.", | ||
| ) | ||
| text = images | ||
| images = None | ||
| if images is None: | ||
| # assuming the user wants to use the old behavior with prompts as the only argument | ||
| prompts = text | ||
| elif text is not None: | ||
| # Assuming image-text-to-text behavior: | ||
| # Check if batched images are provided | ||
| if not isinstance(images, (list, tuple)): | ||
| images = [images] | ||
| if isinstance(text, str): | ||
| # one prompt for all images instead of one prompt per image | ||
| text = [text] * len(images) | ||
| # Check if batched text is provided | ||
| if isinstance(text, (list, tuple)) and len(text) != len(images): | ||
| raise ValueError( | ||
| "When using the image-text-to-text behavior, the number of prompts should be the same as the number of images." | ||
| ) | ||
| # Check that only text is present in the prompts | ||
| if not all(isinstance(i, str) for i in text): | ||
| raise ValueError("When using the image-text-to-text behavior, the prompts should only contain text.") | ||
| prompts = list(zip(images, text)) |
There was a problem hiding this comment.
Lots of logic is needed for backward compatibility, as idefics used to take only prompts where text and images inputs would be interleaved. This added logic preserve supports for these kind of inputs (where prompts is replaced by text arg), while adding support for usual text and images inputs as in other image-text-to-text models. This will also be useful to support idefics in the image-text-to-text pipeline.
| if isinstance(text, str): | ||
| # one prompt for all images instead of one prompt per image | ||
| text = [text] * len(images) |
There was a problem hiding this comment.
I guess this code block is for new processing behavior when users pass images and text.
Not very sure this is a good idea to repeat text several times. Suppose user has one prompt with interleaved images-text, then we would replicate the prompt several times and cause error in downstream modeling code. For ex:
processor(text=["User: What do you see here? Assistant: a cat. User: what about this image?"], images=[image1, image2])
There was a problem hiding this comment.
Yes that's a good point. Although interleaved images-text is not really supported when providing both images and text for Idefics, as there is no way to indicate where to put the images in the prompt. Maybe I should add a warning here instead of automatically duplicating the prompts?
There was a problem hiding this comment.
Ah I see now, indeed Idefics is a bit peculiar.
Yes interleaving like that is not, but providing more than 1 image per prompt like in multi-turn conversation is okey, as in the dosctring of call method. Then we should expect users to pass as many images as prompts, and they would have to wrap images as a batched list if there's more than one per prompt.
I think we can even raise an error, as we cannot know for sure what is the user expecting with these inputs. An error explaining what kind of input we want and let the user fix it, otherwise users who never read warnings might start complaining in the issues :)
There was a problem hiding this comment.
Added support for multiple images per prompt, and this warning to make it clearer what input format we expect when using image-text-to-text format:
transformers/src/transformers/models/idefics/processing_idefics.py
Lines 353 to 358 in 8b171a7
| def test_tokenizer_defaults_preserved_by_kwargs(self): | ||
| if "image_processor" not in self.processor_class.attributes: |
There was a problem hiding this comment.
imo we don't have to overwrite all tests for idefics, seems quite similar to the general test from mixin except for padding="max_length". We can and maybe should indicate padding="max_length" in mixin tests, cause we can't assume all tokenizer will default to "max_length" padding
yonigozlan
force-pushed
the
uniformize-processors-kwargs-idefics-idefics2
branch
from
9799303
to
8b171a7
Compare
What does this PR do?
Adds uniformized processors kwargs following #31911 for the following models:
Before submitting
Pull Request section?
to it if that's the case.
documentation guidelines, and
here are tips on formatting docstrings.
Who can review?
@molbap @zucchini-nlp @amyeroberts